Fix encoding of entity unlock events #462
Conversation
…ures on orchestrators
| { | ||
| P.EntityUnlockSentEvent unlockSentEvent = protoEvent.EntityUnlockSent; | ||
| string name = EncodeEventName(null); | ||
| string name = "release"; |
There was a problem hiding this comment.
I trust that this is correct, but can we get context on it? It seems the other encodes use the same EncodeEventName, why is this one different?
If this has always been wrong, can we also get a test for this path that shows it is working? Or is there already a test that has traditionally been flaky and we haven't investigated it?
There was a problem hiding this comment.
I think maybe this PR adds the tests
Yes, specifically the test in TwoCriticalSections.cs
There was a problem hiding this comment.
I trust that this is correct, but can we get context on it?
The event name must be encoded the same way as it is decoded on line
.The format we use here to encode entity messages as events is the same as in DurableTask.Core (for example, event names are defined in
https://github.com/Azure/durabletask/blob/main/src/DurableTask.Core/Entities/EventFormat/EntityMessageEventNames.cs) but it is not technically required to be the same and we do not want to introduce a dependency on the latter, so the class EntityConversions.cs is designed to be self-contained, and now, hopefully, self-consistent.
This has been just wrong from the beginning.
We eventually noticed this because it causes nondeterminism failures on orchestrators. During replay the events are compared and they don't match because of the incorrect encoding.